Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

model_gateway/src/routers/openai/responses/streaming.rs (1)

1089-1105: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

The orchestrator lookup is still unconditional.

ctx.components.mcp_orchestrator() is resolved before this request_uses_mcp_routing(tools) guard, so a plain non-MCP streaming request still returns internal_error in MCP-disabled deployments. The lookup needs to move into the guarded branch with the format-registry check.

Suggested fix

-    let mcp_orchestrator = match ctx.components.mcp_orchestrator() {
-        Some(m) => m.clone(),
-        None => {
-            return error::internal_error("internal_error", "MCP orchestrator required");
-        }
-    };
     // Only MCP-laden requests need the format registry; plain streaming
     // requests must still pass through deployments without MCP wiring.
     let mcp_routing = match original_body.tools.as_deref() {
         Some(tools) if request_uses_mcp_routing(tools) => {
+            let Some(mcp_orchestrator) = ctx.components.mcp_orchestrator().cloned() else {
+                return error::internal_error("internal_error", "MCP orchestrator required");
+            };
             let Some(registry) = ctx.components.mcp_format_registry() else {
                 return error::internal_error(
                     "internal_error",
                     "MCP format registry required for requests carrying MCP/builtin tools",
                 );
             };
             let registry = registry.clone();
             ensure_request_mcp_client(&mcp_orchestrator, &registry, tools)
                 .await
-                .map(|servers| (servers, registry))
+                .map(|servers| (servers, registry, mcp_orchestrator))
         }
         _ => None,
     };
 
-    let Some((mcp_servers, mcp_format_registry)) = mcp_routing else {
+    let Some((mcp_servers, mcp_format_registry, mcp_orchestrator)) = mcp_routing else {
         return handle_simple_streaming_passthrough(&client, &worker, headers.as_ref(), req).await;
     };

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@model_gateway/src/routers/openai/responses/streaming.rs` around lines 1089 -
1105, Currently ctx.components.mcp_orchestrator() is resolved unconditionally
causing internal errors for non-MCP streaming requests; move the orchestrator
lookup into the branch guarded by request_uses_mcp_routing(tools) and after
confirming ctx.components.mcp_format_registry() exists, so only requests with
MCP/builtin tools call ctx.components.mcp_orchestrator(), then pass that
orchestrator into ensure_request_mcp_client along with the cloned registry and
tools (references: original_body.tools, request_uses_mcp_routing,
mcp_format_registry, mcp_orchestrator, ensure_request_mcp_client).

model_gateway/src/routers/openai/responses/non_streaming.rs (1)

66-80: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

The MCP component lookup is still too eager here.

ctx.components.mcp_orchestrator() is still required before you know the request actually needs MCP routing, so plain non-tool requests can still fail with internal_error instead of taking the direct upstream path. Move the orchestrator lookup into this guarded branch as well.

Suggested fix

-    let mcp_orchestrator = match ctx.components.mcp_orchestrator() {
-        Some(m) => m,
-        None => {
-            return error::internal_error("internal_error", "MCP orchestrator required");
-        }
-    };
-
     // Only MCP-laden requests need the format registry; without this
     // narrowing, plain non-MCP requests would 500 in deployments that run
     // the gateway without MCP wiring. A registry-less MCP request still
     // hard-fails — silent fallback would mis-route hosted tools.
     let mcp_routing = match original_body.tools.as_deref() {
         Some(tools) if request_uses_mcp_routing(tools) => {
+            let Some(mcp_orchestrator) = ctx.components.mcp_orchestrator().cloned() else {
+                return error::internal_error("internal_error", "MCP orchestrator required");
+            };
             let Some(registry) = ctx.components.mcp_format_registry() else {
                 return error::internal_error(
                     "internal_error",
                     "MCP format registry required for requests carrying MCP/builtin tools",
                 );
             };
-            ensure_request_mcp_client(mcp_orchestrator, registry, tools)
+            ensure_request_mcp_client(&mcp_orchestrator, registry, tools)
                 .await
-                .map(|servers| (servers, registry.clone()))
+                .map(|servers| (servers, registry.clone(), mcp_orchestrator))
         }
         _ => None,
     };
 
-    if let Some((mcp_servers, mcp_format_registry)) = mcp_routing {
+    if let Some((mcp_servers, mcp_format_registry, mcp_orchestrator)) = mcp_routing {
         let session_request_id = original_body
             .request_id
             .clone()
             .unwrap_or_else(|| format!("req_{}", uuid::Uuid::now_v7()));

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@model_gateway/src/routers/openai/responses/non_streaming.rs` around lines 66
- 80, The current code eagerly obtains ctx.components.mcp_orchestrator() before
checking whether the request needs MCP routing, which can cause non-tool
requests to error; change the logic so that you only call
ctx.components.mcp_orchestrator() inside the guarded branch where tools are Some
and request_uses_mcp_routing(tools) is true and after confirming
ctx.components.mcp_format_registry() exists; then pass that orchestrator into
ensure_request_mcp_client(...) (instead of using a pre-fetched mcp_orchestrator)
so non-MCP requests can take the direct upstream path without failing.

model_gateway/src/routers/common/openai_bridge/transformer.rs (1)

41-58: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Passthrough failures still discard MCP-provided error text.

When error_message is None, this falls back to "Tool execution failed" and clears McpCall.output, even though ToolExecutionOutput.output can already contain the real MCP error payload. That leaves clients with a generic failure and no actionable detail.

Suggested fix

 fn failed_output_item(
     output: &smg_mcp::ToolExecutionOutput,
     response_format: ResponseFormat,
 ) -> ResponseOutputItem {
     let err_msg = output
         .error_message
         .clone()
-        .unwrap_or_else(|| "Tool execution failed".to_string());
+        .filter(|msg| !msg.is_empty())
+        .or_else(|| {
+            let text = ResponseTransformer::flatten_mcp_output(&output.output);
+            (!text.is_empty()).then_some(text)
+        })
+        .unwrap_or_else(|| "Tool execution failed".to_string());
     match response_format {
         ResponseFormat::Passthrough => ResponseOutputItem::McpCall {
             id: mcp_response_item_id(&output.call_id),
             status: "failed".to_string(),
             approval_request_id: None,
             arguments: output.arguments_str.clone(),
             error: Some(err_msg),
             name: output.tool_name.clone(),
-            output: String::new(),
+            output: ResponseTransformer::flatten_mcp_output(&output.output),
             server_label: output.server_label.clone(),
         },

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@model_gateway/src/routers/common/openai_bridge/transformer.rs` around lines
41 - 58, The Passthrough branch in failed_output_item is discarding the MCP
error payload by setting ResponseOutputItem::McpCall.output to an empty string
and using a generic err_msg when output.error_message.is_none(); change the
logic so failed_output_item (function) uses output.output.clone() (from
ToolExecutionOutput.output) as the McpCall.output when output.error_message is
None (or append it alongside error_message), and keep error set to the concrete
error_message when present (falling back to the generic message only for the
error field), ensuring ResponseOutputItem::McpCall.output carries the
MCP-provided payload instead of being cleared.

model_gateway/src/routers/common/persistence_utils.rs (1)

330-346: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Structural items still don't round-trip in their original shape.

This now stores non-message items whole, but item_to_json() still sends unknown non-message types through the default content branch. A replayed image_generation_call therefore still comes back as {"type":"image_generation_call","content":{...}} instead of restoring top-level fields like revised_prompt and status.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@model_gateway/src/routers/common/persistence_utils.rs` around lines 330 -
346, The persistence path stores non-message items whole (store_whole_item when
item_type != "message") but item_to_json still nests unknown non-message types
under a content field; update the item_to_json logic to return
item_value.clone() unchanged for non-message types instead of falling through to
the default content branch, while keeping the existing message handling (the
branch that wraps { "content", "phase" } when item_type == "message" and phase
is present). Ensure the checks reference the same symbols (item_to_json,
store_whole_item / item_type, and item_value) so replayed structural items like
image_generation_call restore their original top-level fields rather than
becoming {"type": "...", "content": {...}}.

model_gateway/src/routers/common/mcp_utils.rs (1)

250-259: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

request_uses_mcp_routing still doesn't match actual MCP routing.

Line 256 still returns true for any hosted builtin, even when no builtin MCP server is configured, so plain hosted-tool requests can still hard-fail in MCP-disabled deployments instead of falling through upstream. The same helper also stays blind to file_search because extract_builtin_types still omits that family, so configured file_search routing won't activate interception. Please derive this predicate from resolved/configured routing, not builtin presence.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@model_gateway/src/routers/common/mcp_utils.rs` around lines 250 - 259, The
predicate request_uses_mcp_routing is wrong because it returns true for any
hosted builtin via extract_builtin_types and misses file_search; change it to
consult the resolved/configured routing data instead of builtin presence: update
request_uses_mcp_routing to check the actual MCP routing configuration (e.g.,
the routing map or resolved target list the gateway uses at runtime) and return
true only if a tool in tools is explicitly routed to an MCP server (handle
ResponseTool::Mcp(_) as before) or if the resolved routing for known builtin
families (including file_search) points to an MCP endpoint; remove reliance on
extract_builtin_types for this predicate so it matches real configured routing.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes